Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

add NegotiatedCipherSuite to QuicConnection #106368

Closed
wants to merge 1 commit into from
Closed

add NegotiatedCipherSuite to QuicConnection #106368

wants to merge 1 commit into from

Conversation

wfurt
Copy link
Member

@wfurt wfurt commented Aug 13, 2024

contributes to #70184

@wfurt wfurt added NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) area-System.Net.Quic labels Aug 13, 2024
@wfurt wfurt self-assigned this Aug 13, 2024
@dotnet-issue-labeler dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

1 similar comment
@dotnet-issue-labeler dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@dotnet-policy-service Dotnet Policy Service
Copy link
Contributor

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

@wfurt wfurt mentioned this pull request Aug 13, 2024
Closed
4 tasks
@wfurt
Copy link
Member Author

wfurt commented Aug 13, 2024

The server side is failing for no good reason. I opened microsoft/msquic#4457 for tracking.

This was referenced Aug 13, 2024
Open
Open
Open
Open
Closed
rzikm
rzikm reviewed Aug 14, 2024
View reviewed changes
Copy link
Member

@rzikm rzikm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The API proposal also added SslProtocol property, Will you add it as well in this PR?

src/libraries/System.Net.Quic/tests/FunctionalTests/QuicConnectionTests.cs
Comment on lines -50 to -51
Assert.Equal(ApplicationProtocol.ToString(), clientConnection.NegotiatedApplicationProtocol.ToString());
Assert.Equal(ApplicationProtocol.ToString(), serverConnection.NegotiatedApplicationProtocol.ToString());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason for removing these?

@rzikm
Copy link
Member

rzikm commented Aug 14, 2024

I think we need to call the getter proactively during QUIC_CONNECTION_EVENT_COMPLETED as documentation suggests

https://github.com/microsoft/msquic/blob/f96015560399d60cbdd8608b6fa2120560118500/docs/Settings.md#L186-L193

MsQuic drops TLS state after the handshake is complete.

@rzikm rzikm mentioned this pull request Aug 14, 2024
Merged
@wfurt wfurt closed this Aug 14, 2024
@karelz karelz added this to the 9.0.0 milestone Sep 3, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Oct 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@rzikm rzikm rzikm left review comments

Assignees

@wfurt wfurt

Labels
area-System.Net.Quic new-api-needs-documentation NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons)
Projects
None yet
Milestone
9.0.0
Development

Successfully merging this pull request may close these issues.
3 participants
@wfurt @rzikm @karelz